meta: whitelist dotfiles in .gitignore #8016

claudiorodriguez · 2016-08-08T17:00:35Z

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

Description of change

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track.

Fixes: #8012

ChALkeR · 2016-08-08T18:28:08Z

There are a lot for dot-prefixed files in deeper levels.

E.g. in test directory:

./test/addons/.gitignore
./test/.eslintrc
./test/fixtures/.empty-repl-history-file
./test/fixtures/empty/.gitkeep
./test/fixtures/.node_repl_history

silverwind · 2016-08-08T19:02:36Z

There are a lot for dot-prefixed files in deeper levels.

IIRC, filenames without / are recursive by default.

ChALkeR · 2016-08-08T19:05:22Z

@silverwind Exactly. And by blacklisting .* and not whitelisting ./test/fixtures/.empty-repl-history-file by any rule, it gets blacklisted.

silverwind · 2016-08-08T19:17:31Z

It does seem to work, regardless:

$ apply-pr 8016
$ mkdir abc
$ touch abc/.eslintrc
$ touch abc/.dontadd
$ git add --all
$ git status
    new file:   abc/.eslintrc

LGTM once the missing file names are added.

ChALkeR · 2016-08-08T19:27:11Z

@silverwind It works only because those files are either already present in git or present in the whitelist.

Try:

$ git rm ./test/fixtures/.empty-repl-history-file
rm 'test/fixtures/.empty-repl-history-file'
$ touch ./test/fixtures/.empty-repl-history-file
$ git add ./test/fixtures/.empty-repl-history-file
The following paths are ignored by one of your .gitignore files:
test/fixtures/.empty-repl-history-file
Use -f if you really want to add them.

This PR should whitelist all the dot-prefixed-files that we already have in git, not just the top-level ones.

silverwind · 2016-08-08T19:34:56Z

Yes, I agree that's needed. Searching dotfiles in the tree, there might be a few more we ought to include. Or better yet, exclude deps and tools from exclusion.

$ find . -name ".*" | xargs basename | sort | uniq

(partially cleaned up)

.babelrc
.bin
.buildstamp
.clang-format
.deps
.dir-locals.el
.docbuildstamp
.dockerignore
.editorconfig
.empty-repl-history-file
.eslintignore
.eslintrc
.git
.gitattributes
.github
.gitignore
.gitkeep
.gitmodules
.ignore
.jscs.json
.jshintrc
.keep
.lint
.lintignore
.mailmap
.node_repl_history
.npmignore
.remarkrc
.rnd
.tern-port
.tern-project
.testignore
.travis.yml
.ycm_extra_conf.py
.zuul.yml

ChALkeR · 2016-08-08T19:41:38Z

But I don't have .DS_Store (or .dontadd, or .buildstamp, or .docbuildstamp).

Also, I believe that we don't need at least part of those.

ChALkeR · 2016-08-08T19:43:01Z

Btw, perhaps we should just whitelist all dot-prefixed files in the deps dir and use a blacklist if appropriate?

silverwind · 2016-08-08T19:44:03Z

Yeah, those were from my local folder. I cleaned up a few in above list. I think most of the useless files there come from deps/npm. I guess we should go through them on a per-case basis if they are really necessary. I'd wager 90% are not.

silverwind · 2016-08-08T20:08:49Z

I think this should be a more or less complete list of "good" files:

.editorconfig
.eslintignore
.eslintrc
.git
.gitattributes
.github
.gitignore
.gitkeep
.mailmap
.remarkrc

And a list of directories to keep dotfiles from:

deps
test/fixtures
tools/eslint
tools/doc/node_modules

ChALkeR · 2016-08-08T20:20:26Z

.git

Nope =)

imyller · 2016-08-08T20:28:35Z

👍 for this effort!

One thing to consider is that not all IDEs use dotfiles for their files.

For example Netbeans (and it's commercial variants, I'd assume) create nbproject folder.

claudiorodriguez · 2016-08-09T07:35:08Z

@silverwind just updated the file with those, can you take another look?

ChALkeR · 2016-08-09T07:41:27Z

git blame on the lines being deleted:

86eab1f (Ryan Dahl 2009-04-16 13:20:22 +0200 6) .waf*
86eab1f (Ryan Dahl 2009-04-16 13:20:22 +0200 8) .lock-wscript
ff456b3 (Ryan Dahl 2010-10-13 16:20:24 -0700 18) .benchmark_reports
3216f08 (Bert Belder 2010-11-25 01:57:15 +0100 19) /.project
af92315 (Ryan Dahl 2011-03-18 09:14:15 -0700 20) /.cproject
61886fa (Mike Kaufman 2016-04-13 14:38:19 -0700 45) .vs/
1e1bbe5 (Josh Gavant 2016-08-03 14:35:48 -0700 46) .vscode/
c6843f4 (Bert Belder 2012-07-07 23:19:12 +0200 45) .svn/

We should check that nothing of those was targeted towards the files that are being whitelisted now (i.e. deps/, test/, tools/).

claudiorodriguez · 2016-08-09T07:50:17Z

@ChALkeR Except for /.project and /.cproject (the ignore being at root level), I could leave the others in, just in case.

EDIT: I also put the dotfile whitelist at the top of the file so all the other *.<something> ignores apply to the folders

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track. Fixes: nodejs#8012

ChALkeR · 2016-08-09T08:10:11Z

@claudiorodriguez I don't think we should leave those in, we should rather check where those came from.

E.g. .benchmark_reports is definitely not needed, it was being created by the benchmarks at some point in the past, but that doesn't seem to be the case now.

claudiorodriguez · 2016-08-10T07:46:37Z

@ChALkeR fair enough.
@ry @mike-kaufman @joshgav @piscisaureus @nodejs/collaborators anyone able to weigh in?

mike-kaufman · 2016-08-10T15:24:57Z

Line I added was to ignore .vs/ directory. Looks like this will continue with proposed changes, so LGTM.

jasnell · 2016-08-10T16:52:36Z

I haven't tested this out locally just yet so I won't sign off on it but nothing stands out as problematic in the patch.

joshgav · 2016-08-10T16:54:09Z

Replacing .vscode/ as proposed LGTM. Can't say I'm overall in favor of this change though as I don't have a problem with the existing arrangement.

imyller · 2016-08-15T09:39:43Z

Any progress on this?

claudiorodriguez · 2016-08-15T10:28:54Z

@ChALkeR do you think it would make sense to leave them in for now and remove them in future PRs?

@jasnell were you able to test locally?

imyller · 2016-09-24T21:36:27Z

Can we get this PR moving forward? Any objections to landing the proposed changes?

jasnell · 2016-09-26T15:04:27Z

No objections. @nodejs/ctc any objections on this?

sam-github · 2016-10-18T17:41:31Z

I'm rebuilding to confirm, but AFAICT, after doing a make -j8 test, as required for PRs, there are 600+ files listed as ??/unknown. I'm rebuilding again from scratch after a git clean to confirm, but am I doing something wrong? How did it get like this?

Granted, this happened before the change, but if we're improving the .gitignore file, why not make it work?

As for the change to whitelisting, I'm +0 on it. It supports people who haven't added their editor's temp files to their ~/.gitignore (why?) a bit better, and hopefully won't confuse people when adding new files. Lets give it a try, and see if it makes things better.

sam-github · 2016-10-18T19:08:58Z

OK, I take #8016 (comment) it back, I can't repro with a completely clean build, I've no idea what happened.

Trott · 2016-12-02T05:21:36Z

Bump. This seemed poised to land, but never did. I don't feel strongly about this one either way, but I guess if it means we stop having to evaluate (and typically reject) PRs adding this or that dot-file every now and then, I'm mildly in favor.

targos · 2016-12-02T07:16:44Z

.gitignore

@@ -1,3 +1,19 @@
+# Whitelist dotfiles
+.*


Wouldn't it be better to use /.* ? I think that way we wouldn't have to whitelist subdirectories below.

There are meta files from various file managers (e.g. Finder in macOS), IDEs, version control systems, backup systems that can/will create dotfiles in subdirectories.

I think it's easier to whitelist what the Node.js project knows it needs than to react case-by-case to every external product out there.

claudiorodriguez · 2016-12-02T15:39:12Z

I think there's no reason not to land it, I'll land in 24 hours if no one objects.

silverwind · 2016-12-02T15:46:16Z

@claudiorodriguez can you take care of #8016 (comment)? LGTM otherwise.

claudiorodriguez · 2016-12-02T15:53:50Z

@silverwind I didn't remove those since they could still appear in test/fixtures, tools/eslint, or tools/doc/node_modules, and they wouldn't get ignored.

silverwind · 2016-12-03T10:01:52Z

they wouldn't get ignored.

Wouldn't the .* pick those up?

Fishrock123

Applied without issue (other than a merge conflict solves by 3-way merge), tests pass, all files seem to be there, git status is clean, anything else holding this up?

Fishrock123 · 2016-12-05T00:37:14Z

Thanks, landed in 15cc7c0.

And well, if anything explodes I guess I'm biting the bullet ¯\_(ツ)_/¯

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track. Refs: #8010 Refs: #9111 Refs: #10052 Fixes: #8012 PR-URL: #8016 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track. Refs: nodejs#8010 Refs: nodejs#9111 Refs: nodejs#10052 Fixes: nodejs#8012 PR-URL: nodejs#8016 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track. Refs: #8010 Refs: #9111 Refs: #10052 Fixes: #8012 PR-URL: #8016 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Josh Gavant <josh.gavant@outlook.com>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 8, 2016

claudiorodriguez force-pushed the gitignore-whitelist branch from 485f7a8 to 2d8ebc5 Compare August 9, 2016 07:29

meta: whitelist dotfiles in .gitignore

7dc9f9a

Instead of excluding IDE-specific dotfiles, exclude all and then whitelist those the project needs to track. Fixes: nodejs#8012

claudiorodriguez force-pushed the gitignore-whitelist branch from 2d8ebc5 to 7dc9f9a Compare August 9, 2016 07:57

imyller mentioned this pull request Aug 10, 2016

gitignore: add .idea folder to .gitignore #8010

Closed

1 task

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

lpinca mentioned this pull request Sep 27, 2016

stream: improving error msg for stream #8801

Closed

4 tasks

richardlau mentioned this pull request Oct 16, 2016

add IntelliJ project file+directory to .gitignore #9111

Closed

1 task

fhinkel approved these changes Oct 16, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

Trott mentioned this pull request Dec 2, 2016

gitignore: add osx .DS_STORE files #10052

Closed

2 tasks

targos reviewed Dec 2, 2016

View reviewed changes

Fishrock123 approved these changes Dec 3, 2016

View reviewed changes

targos approved these changes Dec 3, 2016

View reviewed changes

Fishrock123 closed this Dec 5, 2016

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

MylesBorins added land-on-v4.x labels Dec 20, 2016

This was referenced Dec 21, 2016

V6.9.3 proposal #10394

Merged

V4.7.1 proposal #10395

Merged

meta: whitelist dotfiles in .gitignore #8016

meta: whitelist dotfiles in .gitignore #8016

Conversation

claudiorodriguez commented Aug 8, 2016

Checklist

Affected core subsystem(s)

Description of change

ChALkeR commented Aug 8, 2016

silverwind commented Aug 8, 2016 • edited Loading

ChALkeR commented Aug 8, 2016

silverwind commented Aug 8, 2016 • edited Loading

ChALkeR commented Aug 8, 2016 • edited Loading

silverwind commented Aug 8, 2016 • edited Loading

ChALkeR commented Aug 8, 2016

ChALkeR commented Aug 8, 2016 • edited Loading

silverwind commented Aug 8, 2016

silverwind commented Aug 8, 2016

ChALkeR commented Aug 8, 2016

imyller commented Aug 8, 2016

claudiorodriguez commented Aug 9, 2016

ChALkeR commented Aug 9, 2016 • edited Loading

claudiorodriguez commented Aug 9, 2016 • edited Loading

ChALkeR commented Aug 9, 2016

claudiorodriguez commented Aug 10, 2016

mike-kaufman commented Aug 10, 2016

jasnell commented Aug 10, 2016

joshgav commented Aug 10, 2016

imyller commented Aug 15, 2016

claudiorodriguez commented Aug 15, 2016

imyller commented Sep 24, 2016

jasnell commented Sep 26, 2016

sam-github commented Oct 18, 2016

sam-github commented Oct 18, 2016

Trott commented Dec 2, 2016

targos Dec 2, 2016

Choose a reason for hiding this comment

imyller Dec 2, 2016

Choose a reason for hiding this comment

claudiorodriguez commented Dec 2, 2016

silverwind commented Dec 2, 2016

claudiorodriguez commented Dec 2, 2016

silverwind commented Dec 3, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 5, 2016 • edited Loading

silverwind commented Aug 8, 2016 •

edited

Loading

silverwind commented Aug 8, 2016 •

edited

Loading

ChALkeR commented Aug 8, 2016 •

edited

Loading

silverwind commented Aug 8, 2016 •

edited

Loading

ChALkeR commented Aug 8, 2016 •

edited

Loading

ChALkeR commented Aug 9, 2016 •

edited

Loading

claudiorodriguez commented Aug 9, 2016 •

edited

Loading

Fishrock123 commented Dec 5, 2016 •

edited

Loading